Module.Callback Refactor#2645
Conversation
…h behaviours
- Add Ash.BehaviourHelpers.call_and_validate_return for Notifier, Change, etc.
- Notifier.notify/1 must return :ok
- Change.atomic/3 validates return (e.g. {:not_atomic, String.t()})
- Fix test Notifiers to return :ok; fix test atomic changes to return {:not_atomic, reason}
- Add behaviour_return_validation_test.exs
Made-with: Cursor
Made-with: Cursor
Resolve managed_relationships merge conflicts while preserving M2M error path/index handling. Made-with: Cursor
| # Only run this job when we're on the main branch, not for PRs | ||
| if: github.ref == 'refs/heads/main' | ||
| # Only run this job for upstream main (forks often lack dependency submission permissions) | ||
| if: github.ref == 'refs/heads/main' && github.repository == 'ash-project/ash' |
There was a problem hiding this comment.
We can't add this restriction because many different repositories use this same CI. We'd need to check maybe just the organization, i.e ash-project
There was a problem hiding this comment.
Okay, undid the new 105-106 and left as original
|
|
||
| def notify(notification) do | ||
| send(self(), {:notification, notification}) | ||
| :ok |
There was a problem hiding this comment.
Given that we weren't requiring this before, I think we should just change the callback to any TBH. Because we've always ignored these results this could break a bunch of people's apps. Alternatively we can add a warn?: true option that will warn on an invalid return and then we can fix it in 4.0 to be more restriction.
| test-subprojects: | ||
| runs-on: ubuntu-latest | ||
| name: Subproject ${{matrix.project.org}}/${{matrix.project.name}} - OTP ${{matrix.otp}} / Elixir ${{matrix.elixir}} | ||
| if: ${{ github.repository == 'ash-project/ash' || matrix.project.name != 'ash_oban' }} |
There was a problem hiding this comment.
Why not run this for ash_oban?
There was a problem hiding this comment.
Thanks for catching this! This isn’t “disable fork CI” wholesale — it only skips the ash_oban subproject matrix job on forks. On ash-project/ash, we still run it. The motivation is that ash_oban’s mix deps.get can pull oban_pro / private Hex packages that forks may not have configured, so the matrix cell would fail for reasons not related to the individuals code. Happy to revisit if we want a different approach 👍
|
Looks like some conflicts (sorry) and then we can get it merged! Great change 🔥 |
|
🚀 Thank you for your contribution! 🚀 |
2 participants
Contributor checklist
Leave anything that you believe does not apply unchecked.